-
-
Notifications
You must be signed in to change notification settings - Fork 12
fix(policy): run pg/sqlite tests, misc dual db compatibility fixes #260
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
|
Note Other AI code review bot(s) detectedCodeRabbit has detected other AI code review bot(s) in this pull request and will avoid duplicating their findings in the review comments. This may lead to a less comprehensive review. WalkthroughAdds CI matrix for sqlite/postgresql; threads client options into result processing and dialects; introduces dialect-based output transforms and SQL-type/casing APIs; converts many parallel relation operations to sequential awaits; centralizes text-matching and updates stdlib signatures; consolidates tests to a createTestClient utility and per-test DB naming. Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~75 minutes Possibly related PRs
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
Claude finished @ymc9's task —— View job PR Review: Database Compatibility FixesI've completed a comprehensive review of this PR focused on dual database compatibility. Here's my detailed analysis: Todo List:
SummaryThis PR successfully implements dual database compatibility for PostgreSQL and SQLite with extensive test coverage. The changes are well-structured and address key differences between the database providers. Code Quality & Best Practices ✅Positive Aspects:
Areas for Improvement:
Database Dialect Implementation 🎯PostgreSQL Dialect (
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull Request Overview
This PR implements dual database compatibility fixes to enable running tests with both PostgreSQL and SQLite providers, addressing miscellaneous compatibility issues.
- Centralizes database provider configuration through environment variables and shared utilities
- Adds proper data type handling and transformation between databases
- Updates SQL queries to use properly quoted identifiers for cross-database compatibility
Reviewed Changes
Copilot reviewed 53 out of 53 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| packages/runtime/test/utils.ts | Consolidates client creation logic and adds dynamic database provider selection based on environment variables |
| packages/runtime/test/query-builder/query-builder.test.ts | Migrates from direct client instantiation to shared test utilities |
| packages/runtime/test/policy/migrated/view.test.ts | Updates SQL view creation with quoted identifiers for database compatibility |
| packages/runtime/test/policy/migrated/toplevel-operations.test.ts | Removes unused import and logger configuration |
| packages/runtime/test/policy/migrated/nested-to-many.test.ts | Updates test data values and adds debug logging |
| packages/runtime/test/policy/migrated/deep-nested.test.ts | Makes error message assertion more generic across databases |
| packages/runtime/test/policy/migrated/auth.test.ts | Makes error message assertion more generic across databases |
| packages/runtime/test/policy/crud/update.test.ts | Fixes test name and updates WHERE clause to use qualified column references |
| packages/runtime/test/policy/basic-schema-read.test.ts | Simplifies test to use shared client utilities and removes database-specific date handling |
| Multiple test files | Standardizes test setup to use createTestClient utility instead of provider-specific implementations |
| packages/runtime/src/plugins/policy/policy-handler.ts | Adds proper type casting for policy evaluation queries |
| packages/runtime/src/plugins/policy/expression-transformer.ts | Improves null comparison handling with proper boolean logic |
| packages/runtime/src/client/result-processor.ts | Refactors to use dialect-specific output transformation |
| packages/runtime/src/client/functions.ts | Adds explicit type casting for string operations |
| packages/runtime/src/client/crud/operations/base.ts | Changes relation processing from parallel to sequential execution |
| packages/runtime/src/client/crud/dialects/*.ts | Implements database-specific type transformations and SQL type mapping |
| packages/runtime/package.json | Adds provider-specific test scripts |
| .github/workflows/build-test.yml | Configures CI to test both database providers |
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 13
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (5)
packages/runtime/test/query-builder/query-builder.test.ts (1)
1-16: Disconnect client after testAdd teardown to release DB resources; otherwise this test can keep connections open.
-import { describe, expect, it } from 'vitest'; +import { afterEach, describe, expect, it } from 'vitest'; @@ - it('works with queries', async () => { - const client = await createTestClient(schema); + it('works with queries', async () => { + const client = await createTestClient(schema); @@ expect(u3).toMatchObject({ email: '[email protected]', role: 'USER' }); }); + + afterEach(async () => { + // best‑effort; disconnect if the test created a client + try { await (global as any).client?.$disconnect?.(); } catch {} + });packages/runtime/src/client/crud/operations/base.ts (2)
1489-1504: Guard m2m connect when target entity is not found to avoid runtime TypeErrorgetEntityIds may return null; handleManyToManyRelation then dereferences fields on null, causing a crash instead of a clean NotFound.
- for (const d of _data) { - const ids = await this.getEntityIds(kysely, model, d); - const r = await this.handleManyToManyRelation( + for (const d of _data) { + const ids = await this.getEntityIds(kysely, model, d); + if (!ids) { + results.push(undefined); + continue; + } + const r = await this.handleManyToManyRelation( kysely, 'connect', fromRelation.model, fromRelation.field, fromRelation.ids, m2m.otherModel!, m2m.otherField!, ids, m2m.joinTable, ); results.push(r); }
1626-1644: Don’t return early on first missing target during m2m disconnectReturning aborts processing subsequent items; silently ignore not‑found entries and continue.
- for (const d of disconnectConditions) { + for (const d of disconnectConditions) { const ids = await this.getEntityIds(kysely, model, d); - if (!ids) { - // not found - return; - } + if (!ids) { + // not found; skip this one + continue; + } await this.handleManyToManyRelation( kysely, 'disconnect', fromRelation.model, fromRelation.field, fromRelation.ids, m2m.otherModel, m2m.otherField, ids, m2m.joinTable, ); - } + }packages/runtime/src/client/functions.ts (2)
70-79: PG-only '&&' used — breaks SQLitehasSome emits eb(field, '&&', search) (Postgres array-overlap); this will error on SQLite — gate by dialect or provide a SQLite fallback (e.g. JSON1/json_each or equivalent IN-based check). File: packages/runtime/src/client/functions.ts (lines 70–79).
48-57: Makehasdialect-aware and avoid passing JS arrays to SQL
- packages/runtime/src/client/functions.ts:48-57 currently does
return eb(field, '@>', [search]);—@>/&&are PostgreSQL‑specific and will break SQLite (this repo targets both).- Passing
[search]produces a JS array parameter, not a SQL ARRAY literal; drivers (and the repo's Postgres dialect) special‑case array SQL (see packages/runtime/src/client/crud/dialects/postgresql.ts:423 and base-dialect.ts:506–511).- Fix: change
hasto accept the ZModelFunctionContext (third param) and use the dialect to build the RHS. For Postgres emit a SQL ARRAY literal (e.g. sqlARRAY[${sql.join(...)}]or reuse the existing Postgres helper); for SQLite implement a JSON1-based contains expression or a dialect helper/fallback. Do not pass raw JS arrays.
🧹 Nitpick comments (33)
packages/runtime/test/client-api/relation/self-relation.test.ts (1)
7-7: Avoid double-running providers: drop describe.each and read provider from envWith the CI matrix now passing TEST_DB_PROVIDER, keeping describe.each runs both providers inside each job, doubling runtime and causing redundant DB work. Read provider from a helper and use a single describe.
Apply:
-import { createTestClient } from '../../utils'; +import { createTestClient, getTestDbProvider } from '../../utils'; @@ -describe.each([{ provider: 'sqlite' as const }, { provider: 'postgresql' as const }])( - 'Self relation tests', - ({ provider }) => { +describe('Self relation tests', () => { + const provider = getTestDbProvider();Also applies to: 2-2, 6-8
packages/runtime/test/client-api/relation/one-to-one.test.ts (1)
7-7: Align with env-driven provider selection; remove per-file provider parameterizationSame duplication issue. Use a single suite and fetch provider from utils.
Apply:
-import { createTestClient } from '../../utils'; +import { createTestClient, getTestDbProvider } from '../../utils'; @@ -describe.each([{ provider: 'sqlite' as const }, { provider: 'postgresql' as const }])( - 'One-to-one relation tests', - ({ provider }) => { +describe('One-to-one relation tests', () => { + const provider = getTestDbProvider();Also applies to: 2-2, 6-8
packages/runtime/package.json (1)
11-12: Good: provider-specific test scripts addedThese help local runs against a single backend.
For parity with "test", consider including typecheck:
-"test:sqlite": "TEST_DB_PROVIDER=sqlite vitest run", -"test:postgresql": "TEST_DB_PROVIDER=postgresql vitest run", +"test:sqlite": "TEST_DB_PROVIDER=sqlite vitest run && pnpm test:typecheck", +"test:postgresql": "TEST_DB_PROVIDER=postgresql vitest run && pnpm test:typecheck",packages/runtime/test/plugin/entity-mutation-hooks.test.ts (2)
367-369: Fix misleading comment: mutation is NOT persistedThe assertion expects zero records, so the comment should reflect rollback/no write.
- // mutation is persisted + // mutation is not persisted
212-218: Avoid relying on array index in hook read-backUsing index 0 couples to ordering. Prefer matching by predicate.
- const afterEntities = await args.loadAfterMutationEntities(); - if ((afterEntities![0] as any).title === 'Post1') { + const afterEntities = await args.loadAfterMutationEntities(); + if (afterEntities!.some((e: any) => e.title === 'Post1')) { post1Intercepted = true; - } - if ((afterEntities![0] as any).title === 'Post2') { + } + if (afterEntities!.some((e: any) => e.title === 'Post2')) { post2Intercepted = true; }packages/runtime/test/client-api/mixin.test.ts (1)
71-79: Make error match case-insensitive for cross-dialect stabilityDifferent providers/drivers vary error casing; regex is more robust.
- ).rejects.toThrow('constraint'); + ).rejects.toThrow(/constraint/i);packages/runtime/test/policy/migrated/deep-nested.test.ts (1)
468-486: Harden duplicate-constraint assertion against casing differencesUse a case-insensitive regex to reduce flakiness across SQLite/PG.
- ).rejects.toThrow('constraint'); + ).rejects.toThrow(/constraint/i);packages/runtime/test/client-api/relation/many-to-many.test.ts (1)
7-7: Include provider in suite title for clearer test outputKeeps CI logs self-descriptive when running per-provider.
-describe.each([{ provider: 'sqlite' as const }, { provider: 'postgresql' as const }])( - 'Many-to-many relation tests', +describe.each([{ provider: 'sqlite' as const }, { provider: 'postgresql' as const }])( + 'Many-to-many relation tests for $provider',packages/runtime/test/policy/migrated/auth.test.ts (1)
539-539: Make error assertion dialect-agnosticUse a case-insensitive regex so both SQLite and Postgres messages pass.
- await expect(db.post.create({ data: { title: 'title' } })).rejects.toThrow('constraint'); + await expect(db.post.create({ data: { title: 'title' } })).rejects.toThrow(/constraint/i);packages/runtime/test/policy/migrated/nested-to-many.test.ts (1)
385-385: Remove debug logging from testsAvoid noisy CI output.
- console.log(r);packages/runtime/test/client-api/count.test.ts (1)
37-39: Confirmselect: truesemantics for countIf
select: trueis an alias for_all, ensure both dialects return a number (not{ _all: number }). Otherwise prefer{ select: { _all: true } }for clarity.packages/runtime/test/client-api/undefined-values.test.ts (2)
7-7: Trim trailing space in suite nameMinor nit: trailing space after “tests”.
Apply:
-describe('Client undefined values tests ', () => { +describe('Client undefined values tests', () => {
22-39: Add a nested-where undefined caseConsider adding a nested relation filter with
undefinedfields to ensure deep pruning is consistent across dialects.packages/runtime/test/client-api/type-coverage.test.ts (1)
83-85: Provider guards are appropriate; add brief rationaleSkipping for SQLite here makes sense due to array support differences. Add a short comment so future readers know it’s intentional.
Also applies to: 129-131
packages/runtime/src/client/crud/dialects/base-dialect.ts (1)
48-51: Make return type explicit for transformOutputReturn type is inferred as unknown; make it explicit for clarity and consistent overrides.
- transformOutput(value: unknown, _type: BuiltinType) { - return value; - } + /** + * Post‑read scalar conversion (e.g., parse JSON, decode bytes, cast numerics). Default: passthrough. + */ + transformOutput(value: unknown, _type: BuiltinType): unknown { + return value; + }packages/runtime/test/client-api/filter.test.ts (2)
6-6: Remove trailing space in describe titleTiny nit: "Client filter tests " has a trailing space.
-describe('Client filter tests ', () => { +describe('Client filter tests', () => {
77-127: Make provider branching exhaustive to surface unsupported providers earlyAdd an else branch (or switch) to fail fast if a new provider is introduced.
- if (client.$schema.provider.type === 'sqlite') { + if (client.$schema.provider.type === 'sqlite') { // ... - } else if (client.$schema.provider.type === 'postgresql') { + } else if (client.$schema.provider.type === 'postgresql') { // ... + } else { + throw new Error(`Unhandled provider: ${client.$schema.provider.type}`); }Also applies to: 127-187
packages/runtime/test/policy/basic-schema-read.test.ts (1)
75-76: Strengthen assertion: verify identity instead of just truthyKeep the simplified flow but assert the returned row matches the created user id for better coverage.
- await expect(authClient.$qb.selectFrom('User').selectAll().executeTakeFirstOrThrow()).toResolveTruthy(); + await expect( + authClient.$qb.selectFrom('User').select(['id']).executeTakeFirstOrThrow() + ).resolves.toMatchObject({ id: user.id });packages/runtime/test/client-api/computed-fields.test.ts (1)
12-29: Prefer referencing columns via ref to avoid treating strings as valuesWhen building computed expressions, pass refs instead of raw strings to ensure correct quoting across dialects.
- upperName: (eb: any) => eb.fn('upper', ['name']), + upperName: (eb: any) => eb.fn('upper', [sql.ref('name')]),packages/runtime/test/utils.ts (3)
18-24: Normalize env var to lowercase for robustnessAccept TEST_DB_PROVIDER in mixed case.
-export function getTestDbProvider() { - const val = process.env['TEST_DB_PROVIDER'] ?? 'sqlite'; +export function getTestDbProvider() { + const val = (process.env['TEST_DB_PROVIDER'] ?? 'sqlite').toLowerCase(); if (!['sqlite', 'postgresql'].includes(val!)) { throw new Error(`Invalid TEST_DB_PROVIDER value: ${val}`); } return val as 'sqlite' | 'postgresql'; }
59-61: Gate noisy logs behind a flagReduce test output by logging only when explicitly enabled.
- console.log(`Using provider: ${provider}, db: ${dbName}`); + if (process.env.VERBOSE_TESTS) { + console.log(`Using provider: ${provider}, db: ${dbName}`); + }
172-193: Postgres DB name derivation — LGTM, with a defensive fallbackCurrent logic is good. Consider a fallback if currentTestName is empty (rare in hooks).
- invariant(testName); + invariant(testName, 'currentTestName is required to derive a unique DB name');packages/runtime/test/client-api/typed-json-fields.test.ts (1)
23-27: Consider avoiding prisma db push for speed unless requireddb push per test is expensive. If typed JSON constraints don't require Prisma schema application, prefer $pushSchema.
Please confirm whether these cases fail without usePrismaPush. If not required, removing it will significantly speed up this suite.
packages/runtime/test/client-api/raw-query.test.ts (2)
39-45: Avoid ts-ignore by checking the dialect provider instead of schemaUse the runtime dialect’s provider to select placeholder style; this removes the need for ts-ignore.
- const sql = - // @ts-ignore - client.$schema.provider.type === 'postgresql' + const sql = + // runtime provider check avoids TS narrowing issues + (client.$options.dialect as any).provider === 'postgresql' ? `UPDATE "User" SET "email" = $1 WHERE "id" = $2` : `UPDATE "User" SET "email" = ? WHERE "id" = ?`;
71-77: Apply the same dialect-provider check hereMirror the change for queryRawUnsafe.
- const sql = - // @ts-ignore - client.$schema.provider.type === 'postgresql' + const sql = + (client.$options.dialect as any).provider === 'postgresql' ? `SELECT "User"."id", "User"."email" FROM "User" WHERE "User"."id" = $1` : `SELECT "User"."id", "User"."email" FROM "User" WHERE "User"."id" = ?`;packages/runtime/test/client-api/find.test.ts (1)
676-698: Consider extracting the provider check to reduce code duplication.The provider type check is repeated in multiple places. Consider extracting this into a helper function or using a more declarative approach.
+const isPostgreSQL = (client: ClientContract<typeof schema>) => + client.$schema.provider.type === 'postgresql'; // @ts-ignore -if (client.$schema.provider.type === 'postgresql') { +if (isPostgreSQL(client)) {packages/runtime/src/client/crud/dialects/postgresql.ts (1)
74-83: Consider handling edge cases for BigInt conversion.While the BigInt transformation handles string and number inputs, consider adding validation for numeric string formats and potential overflow scenarios.
private transformOutputBigInt(value: unknown) { if (typeof value === 'bigint') { return value; } invariant( typeof value === 'string' || typeof value === 'number', `Expected string or number, got ${typeof value}`, ); + // Validate string format if it's a string + if (typeof value === 'string' && !/^-?\d+$/.test(value.trim())) { + throw new QueryError(`Invalid BigInt string format: ${value}`); + } return BigInt(value); }packages/runtime/test/client-api/delegate.test.ts (1)
785-793: Standardize and document polymorphic-model "limit" error messages.Found: packages/runtime/src/client/crud/operations/base.ts throws "Updating with a limit is not supported for polymorphic models" and "Deleting with a limit is not supported for polymorphic models"; test asserts update message in packages/runtime/test/client-api/delegate.test.ts. No README/docs reference detected.
Actions:
- Standardize wording across operations (suggest canonical form: "The with a limit is not supported for polymorphic models") and align the "skipDuplicates" message grammar/quoting in the same file.
- Add a short docs entry (README or developer docs) listing this polymorphic-model limitation and affected operations.
- Update tests if the exact error strings are changed.
packages/runtime/src/client/crud/dialects/sqlite.ts (2)
45-51: Avoid mixed storage formats for DateTime on writesNumbers currently pass through unchanged, while strings and Dates become ISO strings. That can lead to mixed TEXT/NUMERIC representations in the same column and inconsistent comparisons. Recommend normalizing numeric inputs to ISO strings too.
Apply:
- .with('DateTime', () => - value instanceof Date - ? value.toISOString() - : typeof value === 'string' - ? new Date(value).toISOString() - : value, - ) + .with('DateTime', () => + value instanceof Date + ? value.toISOString() + : typeof value === 'string' + ? new Date(value).toISOString() + : typeof value === 'number' + ? new Date(value).toISOString() + : value, + )If you prefer storing epoch numbers in SQLite, we should instead coerce everything (Date/string) to a number and align getFieldSqlType accordingly. Confirm the intended representation and we’ll adjust both paths.
98-100: Broaden boolean normalization for common SQLite representationsbetter-sqlite3 often returns 0/1; sometimes strings “0”/“1” appear via views/expressions. Handle these explicitly to avoid surprises.
Apply:
- private transformOutputBoolean(value: unknown) { - return !!value; - } + private transformOutputBoolean(value: unknown) { + if (typeof value === 'number') return value !== 0; + if (typeof value === 'string') return value === '1' || value.toLowerCase() === 'true'; + return !!value; + }packages/runtime/src/client/functions.ts (3)
81-87: Null semantics in isEmpty: consider COALESCE for predictable behaviorAs written, NULL arrays yield NULL = 0 (unknown/false). If "null means empty" is desired, coalesce to 0.
- return eb(dialect.buildArrayLength(eb, field), '=', sql.lit(0)); + return eb(eb.fn('coalesce', [dialect.buildArrayLength(eb, field), sql.lit(0)]), '=', sql.lit(0));
109-124: Typo: "casting" → "casing" in error messageMinor copy fix.
- invariant(ValueNode.is(opNode) && typeof opNode.value === 'string', '"casting" parameter must be a string value'); + invariant(ValueNode.is(opNode) && typeof opNode.value === 'string', '"casing" parameter must be a string value');
1-1: Add the readBoolean helperPlace this helper near processCasing or above the exports to support the proposed changes.
function readBoolean(expr: Expression<any> | undefined, paramName: string): boolean { if (!expr) return false; const node = expr.toOperationNode(); invariant(ValueNode.is(node) && typeof node.value === 'boolean', `"${paramName}" parameter must be a boolean literal`); return node.value; }
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (53)
.github/workflows/build-test.yml(2 hunks)packages/common-helpers/src/index.ts(1 hunks)packages/common-helpers/src/promise-utils.ts(1 hunks)packages/runtime/package.json(1 hunks)packages/runtime/src/client/client-impl.ts(2 hunks)packages/runtime/src/client/crud/dialects/base-dialect.ts(2 hunks)packages/runtime/src/client/crud/dialects/postgresql.ts(5 hunks)packages/runtime/src/client/crud/dialects/sqlite.ts(5 hunks)packages/runtime/src/client/crud/operations/base.ts(10 hunks)packages/runtime/src/client/functions.ts(4 hunks)packages/runtime/src/client/result-processor.ts(3 hunks)packages/runtime/src/plugins/policy/expression-transformer.ts(1 hunks)packages/runtime/src/plugins/policy/policy-handler.ts(4 hunks)packages/runtime/test/client-api/aggregate.test.ts(1 hunks)packages/runtime/test/client-api/client-specs.ts(0 hunks)packages/runtime/test/client-api/computed-fields.test.ts(4 hunks)packages/runtime/test/client-api/count.test.ts(1 hunks)packages/runtime/test/client-api/create-many-and-return.test.ts(1 hunks)packages/runtime/test/client-api/create-many.test.ts(1 hunks)packages/runtime/test/client-api/create.test.ts(1 hunks)packages/runtime/test/client-api/delegate.test.ts(1 hunks)packages/runtime/test/client-api/delete-many.test.ts(1 hunks)packages/runtime/test/client-api/delete.test.ts(1 hunks)packages/runtime/test/client-api/filter.test.ts(3 hunks)packages/runtime/test/client-api/find.test.ts(5 hunks)packages/runtime/test/client-api/group-by.test.ts(1 hunks)packages/runtime/test/client-api/mixin.test.ts(1 hunks)packages/runtime/test/client-api/name-mapping.test.ts(2 hunks)packages/runtime/test/client-api/raw-query.test.ts(3 hunks)packages/runtime/test/client-api/relation/many-to-many.test.ts(1 hunks)packages/runtime/test/client-api/relation/one-to-many.test.ts(1 hunks)packages/runtime/test/client-api/relation/one-to-one.test.ts(1 hunks)packages/runtime/test/client-api/relation/self-relation.test.ts(1 hunks)packages/runtime/test/client-api/scalar-list.test.ts(0 hunks)packages/runtime/test/client-api/transaction.test.ts(1 hunks)packages/runtime/test/client-api/type-coverage.test.ts(3 hunks)packages/runtime/test/client-api/typed-json-fields.test.ts(2 hunks)packages/runtime/test/client-api/undefined-values.test.ts(1 hunks)packages/runtime/test/client-api/update-many.test.ts(1 hunks)packages/runtime/test/client-api/update.test.ts(1 hunks)packages/runtime/test/client-api/upsert.test.ts(1 hunks)packages/runtime/test/plugin/entity-mutation-hooks.test.ts(1 hunks)packages/runtime/test/plugin/on-kysely-query.test.ts(1 hunks)packages/runtime/test/plugin/on-query-hooks.test.ts(1 hunks)packages/runtime/test/policy/basic-schema-read.test.ts(2 hunks)packages/runtime/test/policy/crud/update.test.ts(3 hunks)packages/runtime/test/policy/migrated/auth.test.ts(1 hunks)packages/runtime/test/policy/migrated/deep-nested.test.ts(1 hunks)packages/runtime/test/policy/migrated/nested-to-many.test.ts(2 hunks)packages/runtime/test/policy/migrated/toplevel-operations.test.ts(0 hunks)packages/runtime/test/policy/migrated/view.test.ts(1 hunks)packages/runtime/test/query-builder/query-builder.test.ts(1 hunks)packages/runtime/test/utils.ts(3 hunks)
💤 Files with no reviewable changes (3)
- packages/runtime/test/client-api/scalar-list.test.ts
- packages/runtime/test/policy/migrated/toplevel-operations.test.ts
- packages/runtime/test/client-api/client-specs.ts
🧰 Additional context used
📓 Path-based instructions (1)
{packages,samples,tests}/**
📄 CodeRabbit inference engine (CLAUDE.md)
Place packages only under
packages/,samples/, ortests/
Files:
packages/common-helpers/src/index.tspackages/runtime/test/policy/migrated/deep-nested.test.tspackages/common-helpers/src/promise-utils.tspackages/runtime/test/client-api/upsert.test.tspackages/runtime/test/policy/migrated/view.test.tspackages/runtime/test/client-api/relation/one-to-many.test.tspackages/runtime/test/client-api/relation/one-to-one.test.tspackages/runtime/test/policy/migrated/auth.test.tspackages/runtime/test/client-api/group-by.test.tspackages/runtime/test/plugin/on-query-hooks.test.tspackages/runtime/test/plugin/entity-mutation-hooks.test.tspackages/runtime/src/client/client-impl.tspackages/runtime/src/client/result-processor.tspackages/runtime/src/client/crud/dialects/base-dialect.tspackages/runtime/test/client-api/count.test.tspackages/runtime/test/query-builder/query-builder.test.tspackages/runtime/package.jsonpackages/runtime/test/client-api/transaction.test.tspackages/runtime/test/client-api/raw-query.test.tspackages/runtime/test/client-api/delete-many.test.tspackages/runtime/test/client-api/aggregate.test.tspackages/runtime/test/utils.tspackages/runtime/test/client-api/undefined-values.test.tspackages/runtime/test/client-api/create.test.tspackages/runtime/test/policy/migrated/nested-to-many.test.tspackages/runtime/test/client-api/typed-json-fields.test.tspackages/runtime/test/client-api/update.test.tspackages/runtime/test/client-api/filter.test.tspackages/runtime/test/client-api/relation/many-to-many.test.tspackages/runtime/test/client-api/delete.test.tspackages/runtime/test/client-api/computed-fields.test.tspackages/runtime/src/client/crud/dialects/postgresql.tspackages/runtime/test/client-api/relation/self-relation.test.tspackages/runtime/test/client-api/create-many-and-return.test.tspackages/runtime/test/client-api/find.test.tspackages/runtime/src/client/functions.tspackages/runtime/test/client-api/update-many.test.tspackages/runtime/src/plugins/policy/expression-transformer.tspackages/runtime/test/client-api/mixin.test.tspackages/runtime/test/client-api/name-mapping.test.tspackages/runtime/test/policy/crud/update.test.tspackages/runtime/test/client-api/delegate.test.tspackages/runtime/test/client-api/create-many.test.tspackages/runtime/test/plugin/on-kysely-query.test.tspackages/runtime/src/plugins/policy/policy-handler.tspackages/runtime/test/client-api/type-coverage.test.tspackages/runtime/src/client/crud/operations/base.tspackages/runtime/test/policy/basic-schema-read.test.tspackages/runtime/src/client/crud/dialects/sqlite.ts
🧬 Code graph analysis (36)
packages/runtime/test/client-api/upsert.test.ts (2)
packages/runtime/test/schemas/basic/schema.ts (1)
schema(9-284)packages/runtime/test/utils.ts (1)
createTestClient(50-166)
packages/runtime/test/client-api/group-by.test.ts (2)
packages/runtime/test/schemas/basic/schema.ts (1)
schema(9-284)packages/runtime/test/utils.ts (1)
createTestClient(50-166)
packages/runtime/test/plugin/on-query-hooks.test.ts (2)
packages/runtime/src/client/contract.ts (1)
ClientContract(50-159)packages/runtime/test/utils.ts (1)
createTestClient(50-166)
packages/runtime/test/plugin/entity-mutation-hooks.test.ts (2)
packages/runtime/src/client/contract.ts (1)
ClientContract(50-159)packages/runtime/test/utils.ts (1)
createTestClient(50-166)
packages/runtime/src/client/client-impl.ts (3)
packages/runtime/src/client/crud/operations/base.ts (1)
options(95-97)packages/runtime/src/client/executor/zenstack-query-executor.ts (1)
options(64-66)packages/runtime/src/client/result-processor.ts (1)
ResultProcessor(8-143)
packages/runtime/src/client/result-processor.ts (3)
packages/sdk/src/schema/schema.ts (1)
SchemaDef(10-18)packages/runtime/src/client/options.ts (1)
ClientOptions(45-80)packages/runtime/src/client/crud/dialects/index.ts (1)
getCrudDialect(8-16)
packages/runtime/src/client/crud/dialects/base-dialect.ts (1)
packages/sdk/src/schema/schema.ts (1)
FieldDef(60-75)
packages/runtime/test/client-api/count.test.ts (1)
packages/runtime/test/utils.ts (1)
createTestClient(50-166)
packages/runtime/test/query-builder/query-builder.test.ts (2)
packages/runtime/test/schemas/basic/helper.ts (1)
getSchema(4-11)packages/runtime/test/utils.ts (1)
createTestClient(50-166)
packages/runtime/test/client-api/transaction.test.ts (1)
packages/runtime/test/utils.ts (1)
createTestClient(50-166)
packages/runtime/test/client-api/raw-query.test.ts (2)
packages/runtime/src/client/contract.ts (1)
ClientContract(50-159)packages/runtime/test/utils.ts (1)
createTestClient(50-166)
packages/runtime/test/client-api/delete-many.test.ts (2)
packages/runtime/src/client/contract.ts (1)
ClientContract(50-159)packages/runtime/test/utils.ts (1)
createTestClient(50-166)
packages/runtime/test/client-api/aggregate.test.ts (1)
packages/runtime/test/utils.ts (1)
createTestClient(50-166)
packages/runtime/test/utils.ts (3)
packages/runtime/src/client/crud/dialects/postgresql.ts (1)
provider(33-35)packages/runtime/src/client/crud/dialects/sqlite.ts (1)
provider(27-29)packages/common-helpers/src/tiny-invariant.ts (1)
invariant(4-14)
packages/runtime/test/client-api/undefined-values.test.ts (2)
packages/runtime/test/schemas/basic/schema.ts (1)
schema(9-284)packages/runtime/test/utils.ts (1)
createTestClient(50-166)
packages/runtime/test/client-api/create.test.ts (2)
packages/runtime/test/schemas/basic/schema.ts (1)
schema(9-284)packages/runtime/test/utils.ts (1)
createTestClient(50-166)
packages/runtime/test/client-api/typed-json-fields.test.ts (1)
packages/runtime/test/utils.ts (1)
createTestClient(50-166)
packages/runtime/test/client-api/update.test.ts (1)
packages/runtime/test/utils.ts (1)
createTestClient(50-166)
packages/runtime/test/client-api/filter.test.ts (3)
packages/runtime/src/client/contract.ts (1)
ClientContract(50-159)packages/runtime/test/schemas/basic/schema.ts (1)
schema(9-284)packages/runtime/test/utils.ts (1)
createTestClient(50-166)
packages/runtime/test/client-api/delete.test.ts (2)
packages/runtime/test/schemas/basic/schema.ts (1)
schema(9-284)packages/runtime/test/utils.ts (1)
createTestClient(50-166)
packages/runtime/test/client-api/computed-fields.test.ts (1)
packages/runtime/test/utils.ts (1)
createTestClient(50-166)
packages/runtime/src/client/crud/dialects/postgresql.ts (4)
packages/runtime/src/client/options.ts (1)
ClientOptions(45-80)packages/sdk/src/schema/schema.ts (2)
BuiltinType(85-95)FieldDef(60-75)packages/common-helpers/src/tiny-invariant.ts (1)
invariant(4-14)packages/runtime/src/client/errors.ts (1)
QueryError(13-17)
packages/runtime/test/client-api/create-many-and-return.test.ts (2)
packages/runtime/test/schemas/basic/schema.ts (1)
schema(9-284)packages/runtime/test/utils.ts (1)
createTestClient(50-166)
packages/runtime/test/client-api/find.test.ts (2)
packages/runtime/src/client/contract.ts (1)
ClientContract(50-159)packages/runtime/test/utils.ts (1)
createTestClient(50-166)
packages/runtime/src/client/functions.ts (1)
packages/runtime/src/client/options.ts (2)
ZModelFunction(36-40)ZModelFunctionContext(9-34)
packages/runtime/test/client-api/update-many.test.ts (1)
packages/runtime/test/utils.ts (1)
createTestClient(50-166)
packages/runtime/src/plugins/policy/expression-transformer.ts (3)
packages/sdk/src/schema/expression.ts (1)
BinaryOperator(61-61)packages/common-helpers/src/tiny-invariant.ts (1)
invariant(4-14)packages/runtime/src/plugins/policy/utils.ts (2)
trueNode(21-23)falseNode(28-30)
packages/runtime/test/client-api/name-mapping.test.ts (1)
packages/runtime/test/utils.ts (1)
createTestClient(50-166)
packages/runtime/test/client-api/delegate.test.ts (1)
packages/runtime/test/utils.ts (1)
createTestClient(50-166)
packages/runtime/test/client-api/create-many.test.ts (2)
packages/runtime/test/schemas/basic/schema.ts (1)
schema(9-284)packages/runtime/test/utils.ts (1)
createTestClient(50-166)
packages/runtime/test/plugin/on-kysely-query.test.ts (2)
packages/runtime/src/client/contract.ts (1)
ClientContract(50-159)packages/runtime/test/utils.ts (1)
createTestClient(50-166)
packages/runtime/src/plugins/policy/policy-handler.ts (3)
packages/runtime/src/client/crud/operations/base.ts (1)
requireModel(114-116)packages/runtime/src/client/query-utils.ts (1)
requireModel(25-31)packages/runtime/src/client/index.ts (1)
sql(9-9)
packages/runtime/test/client-api/type-coverage.test.ts (1)
packages/runtime/test/utils.ts (1)
getTestDbProvider(18-24)
packages/runtime/src/client/crud/operations/base.ts (2)
packages/runtime/src/utils/enumerate.ts (1)
enumerate(9-17)packages/common-helpers/src/tiny-invariant.ts (1)
invariant(4-14)
packages/runtime/test/policy/basic-schema-read.test.ts (1)
packages/runtime/test/utils.ts (1)
createTestClient(50-166)
packages/runtime/src/client/crud/dialects/sqlite.ts (3)
packages/sdk/src/schema/schema.ts (2)
BuiltinType(85-95)FieldDef(60-75)packages/common-helpers/src/tiny-invariant.ts (1)
invariant(4-14)packages/runtime/src/client/errors.ts (1)
QueryError(13-17)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: build-test (20.x, postgresql)
- GitHub Check: claude-review
- GitHub Check: build-test (20.x, sqlite)
🔇 Additional comments (66)
packages/common-helpers/src/index.ts (1)
4-4: Re-export looks goodPublic surface extended with promise-utils as expected.
packages/runtime/src/plugins/policy/policy-handler.ts (1)
22-22: Type-aware pre-create policy eval via CAST and relation-field exclusion looks correct
- Excluding relation fields from const table avoids invalid projections.
- Casting VALUES columns to dialect SQL types before aliasing by field name should stabilize comparisons across providers.
Please confirm:
- getFieldSqlType handles array types and provider-specific types (e.g., JSON/Bytes/Decimal) for both sqlite and postgres.
- No policies reference only-defaulted fields where null during pre-check would cause false negatives.
If helpful, I can add targeted tests covering create policies on JSON/Decimal/Array fields.
Also applies to: 366-399
packages/runtime/src/plugins/policy/expression-transformer.ts (1)
199-205: Correct null-comparison semantics via IS/IS NOT and literal foldingSwitching to a dedicated transformNullCheck with IS/IS NOT and literal true/false for ValueNode nulls fixes edge cases for ==/!= null.
Also applies to: 207-220
packages/runtime/test/plugin/entity-mutation-hooks.test.ts (2)
10-16: Per-test client lifecycle looks solidFresh client per test and guaranteed disconnect minimizes cross-test leakage.
289-349: Great coverage of transactional hook semanticsClear separation of out-of-tx vs in-tx behavior and side-effects; assertions match intended guarantees.
packages/runtime/test/client-api/upsert.test.ts (1)
6-11: LGTM: unified, provider-agnostic harnessSwitch to createTestClient with env-driven provider aligns with the new matrix strategy.
packages/runtime/test/client-api/update-many.test.ts (1)
4-4: Switch to unified createTestClient — LGTMCentralized setup improves dual-DB runs and consistency.
Also applies to: 6-6, 10-10
packages/runtime/test/policy/migrated/nested-to-many.test.ts (1)
369-369: Test payload tweak — LGTMUsing one allowed and one denied value keeps the policy assertion meaningful.
packages/runtime/test/client-api/transaction.test.ts (1)
4-4: Refactor to createTestClient — LGTMMatches the new unified test harness and dual-DB strategy.
Also applies to: 6-6, 10-10
packages/runtime/test/client-api/relation/one-to-many.test.ts (1)
7-7: Suite title change — LGTMProvider-agnostic label is fine given describe.each still iterates providers.
packages/runtime/test/plugin/on-query-hooks.test.ts (1)
2-2: Move to createTestClient and adjust imports — LGTMSetup is cleaner; no explicit $pushSchema needed.
Also applies to: 4-4, 10-10
packages/runtime/test/client-api/delete.test.ts (1)
4-4: Adopt unified client factory — LGTMConsistent with the rest of the suite.
Also applies to: 6-6, 10-10
packages/runtime/test/policy/crud/update.test.ts (2)
159-159: Test description clarification — LGTMNaming now correctly reflects the non-owner side check.
1245-1245: Qualify column in ON CONFLICT WHERE — LGTMUsing 'Foo.id' improves cross-dialect query generation.
Also applies to: 1256-1256
packages/runtime/test/client-api/delete-many.test.ts (2)
4-4: Unified setup LGTMAdopting
createTestClient(schema)and a single describe block is consistent with the new dual‑DB test strategy.Also applies to: 6-6, 10-10
54-76: Verify cross-dialect semantics forlimiton deleteMany
DELETE ... LIMITis not native on PostgreSQL; it usually requires a CTE/subquery emulation. Please confirm the dialect layer normalizes behavior so PG and SQLite both return accuratecount.packages/runtime/test/client-api/undefined-values.test.ts (1)
4-4: Migration looks goodUsing
createTestClient(schema)and explicit disconnects is correct.Also applies to: 7-7, 11-11
packages/runtime/test/client-api/type-coverage.test.ts (1)
3-3: Type coverage harness update is soundImporting
getTestDbProviderand consolidating under a single suite is aligned with the new matrix flow.Also applies to: 5-5
packages/runtime/test/client-api/create-many-and-return.test.ts (2)
4-4: Solid move tocreateTestClientSetup/teardown pattern is clean and consistent with the updated utils.
Also applies to: 6-6, 10-10
43-59: Confirm RETURNING/skipDuplicates parity across DBs
- PG: likely uses
ON CONFLICT DO NOTHING RETURNING *.- SQLite:
INSERT OR IGNOREhistorically lackedRETURNINGon older versions.Please confirm the dialect layer returns only inserted rows for both providers and CI runs both paths.
packages/runtime/test/client-api/count.test.ts (1)
4-4: Approve — migration to unified test harness looks correctcreateTestClient(schema) single-suite setup aligns with the new utils.
Run to confirm no leftover uses of deprecated helpers remain:
rg -n --type=ts -C2 '\bcreateClient\s*\(|\bcreateClientSpecs\s*\(|PG_DB_NAME'packages/runtime/test/client-api/aggregate.test.ts (1)
5-12: Switch to createTestClient + proper teardown: LGTMInitialization and cleanup look correct across providers.
packages/runtime/test/client-api/group-by.test.ts (1)
4-12: Unified client factory adoption: LGTMUses createTestClient(schema) with disconnect in afterEach; matches the new test harness.
packages/runtime/test/client-api/update.test.ts (1)
4-12: Client factory migration: LGTMSetup/teardown align with the dual‑DB test strategy.
packages/runtime/src/client/client-impl.ts (2)
327-331: ResultProcessor now uses options (dialect): LGTMPassing options into ResultProcessor is correct for dialect‑aware output transformation.
64-69: No change required — options are always provided by callersRepo search shows every new ZenStackClient(schema, ...) call supplies an options object (e.g. packages/runtime/test/utils.ts:153; samples/blog/main.ts:7; packages/cli/src/actions/generate.ts:39). No single-argument invocations found.
packages/runtime/test/client-api/create-many.test.ts (1)
4-11: Client factory + teardown usage: LGTMLooks consistent with the new unified test utils.
packages/runtime/src/client/crud/dialects/base-dialect.ts (1)
1262-1266: getFieldSqlType implemented in concrete dialects — no action requiredPostgresCrudDialect overrides getFieldSqlType (packages/runtime/src/client/crud/dialects/postgresql.ts:431). SqliteCrudDialect overrides getFieldSqlType (packages/runtime/src/client/crud/dialects/sqlite.ts:374).
packages/runtime/test/client-api/filter.test.ts (2)
4-4: Migration to createTestClient looks goodUsing the unified test client improves consistency across providers.
10-10: Initialize client via createTestClient(schema) — LGTMSetup is clear and provider‑agnostic.
packages/runtime/test/policy/basic-schema-read.test.ts (2)
5-5: Unified client helper import — LGTM
11-11: Client initialization change — LGTMpackages/runtime/test/client-api/computed-fields.test.ts (2)
100-138: Type‑check scenario setup — LGTMCompiles through generateTsSchema with ts‑expect‑error guards.
If any flakiness arises, confirm extraSourceFiles compilation failures do bubble up by intentionally breaking the snippet to see the test fail.
206-244: Relation-based computed field expression — LGTMUsage of whereRef/sql.ref and aggregation is sound.
packages/runtime/test/client-api/typed-json-fields.test.ts (2)
33-125: Test scenarios — LGTMCreate cases cover null default, optional property, extra fields, and invalid payloads.
165-213: Update scenarios — LGTMCovers both permissive extra fields and required-id validation.
packages/runtime/test/client-api/create.test.ts (2)
4-4: Unified test client import — LGTM
6-11: Client init via createTestClient — LGTMpackages/runtime/src/client/crud/operations/base.ts (3)
365-368: Sequentializing post-create relation processing — LGTMThis avoids subtle FK and transaction ordering issues across dialects.
Please sanity‑check create depth-2+ nesting (e.g., A.create -> B.create -> C.create) for both providers to ensure no ordering regressions.
1365-1417: Sequential relation updates — LGTMClearer control flow and easier error propagation.
113-185: Wrap read execution with QueryError including compiled SQL — LGTMImproves diagnosability across dialects.
packages/runtime/test/client-api/find.test.ts (4)
5-5: LGTM! Test utilities properly refactored.The import change from
./client-specsto../utilsaligns with the broader test infrastructure overhaul, consolidating test utilities into a single module.
8-13: LGTM! Consistent test setup with unified client creation.The simplified test structure with a single
describeblock and the use ofcreateTestClient(schema)follows the new pattern established across the test suite. This removes provider-specific test scaffolding in favor of runtime-determined provider configuration.
903-924: LGTM! Proper use of toMatchObject for flexible assertions.The updated assertions using
toMatchObjectwith specific fields provide better test resilience while maintaining proper validation of the response structure.
266-269: Verify SQLite 'distinct' limitation handling and documentationSearch found no other references; confirm the SQLite-specific rejection in packages/runtime/test/client-api/find.test.ts (lines 266–269) is enforced across code, tests, and docs — add a central check or documentation note if missing.
packages/runtime/test/client-api/name-mapping.test.ts (2)
11-16: LGTM! Consistent test setup pattern with Prisma push.The test setup follows the established pattern with the additional
usePrismaPush: trueoption and schema file path, appropriately used for name mapping tests that require specific database schema generation.
22-77: LGTM! Comprehensive name mapping test coverage.The create tests properly validate both ORM operations and raw Kysely query builder operations, ensuring name mapping works correctly across different query interfaces.
packages/runtime/src/client/result-processor.ts (3)
9-15: LGTM! Clean dialect integration pattern.The introduction of the dialect field and its initialization through
getCrudDialectproperly implements the separation of concerns, delegating database-specific transformations to the appropriate dialect implementation.
52-52: LGTM! Proper delegation to dialect for JSON transformation.The change to use
this.dialect.transformOutput(value, 'Json')for delegate joined fields maintains consistency with the new dialect-based transformation approach.
96-99: LGTM! Consistent transformation delegation.The field value processing correctly delegates all transformations to the dialect layer, supporting both array and scalar values uniformly.
packages/runtime/test/client-api/delegate.test.ts (3)
7-23: LGTM! Well-structured delegate model test suite.The test setup follows the established pattern with proper lifecycle management and appropriate use of Prisma push for delegate model testing.
166-190: LGTM! Good atomicity validation.The test properly validates that failed delegate model creation maintains data consistency across all related tables, ensuring atomic transaction behavior.
305-547: LGTM! Comprehensive filter test coverage.The filter tests thoroughly cover various scenarios including toplevel filters, relation filters, parent filters, and relation-based filters from base models, ensuring robust delegate model filtering capabilities.
packages/runtime/src/client/crud/dialects/postgresql.ts (3)
29-31: LGTM! Constructor properly passes options to base class.The constructor correctly forwards both schema and options to the base dialect, enabling access to client configuration in dialect implementations.
65-72: LGTM! Well-structured output transformation with pattern matching.The
transformOutputmethod uses a clean pattern matching approach to delegate type-specific transformations, providing clear and maintainable code.
431-462: LGTM! Comprehensive SQL type mapping.The
getFieldSqlTypemethod provides complete coverage of all supported types with proper array handling. The error for relation fields prevents misuse.packages/runtime/src/client/crud/dialects/sqlite.ts (7)
2-2: Correct: value import for DecimalRuntime helpers instantiate Decimal, so a value import is required. LGTM.
12-12: Type-only imports are appropriateOnly types are consumed from schema; this keeps the runtime lean. LGTM.
15-15: Importing QueryError is appropriateUsed for SQL type guard errors below. LGTM.
76-85: Decimal output normalization looks goodAccepts string/number/Decimal and returns Decimal instance. LGTM.
87-96: BigInt output normalization looks goodCovers string/number inputs and preserves bigint. LGTM.
102-110: Confirm numeric epoch unit for DateTimeThis converts numbers to Date using millisecond epoch. If inputs are seconds, results will be 1000x off. Either document ms expectation or normalize seconds.
I can add a small heuristic (e.g., treat < 10^12 as seconds) if desired.
112-114: Bytes normalization is correctConverts Node Buffer to Uint8Array; preserves other values. LGTM.
packages/runtime/src/client/functions.ts (3)
89-89: now(): LGTMCross‑dialect friendly.
91-98: currentModel(): LGTMSignature/context usage and casing support look good.
100-107: currentOperation(): LGTMConsistent with currentModel.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
packages/runtime/test/policy/policy-functions.test.ts (1)
235-236: Policy/time logic likely inverted; update should not be rejectedPolicy is
@@allow('update', now() >= dt). Settingdtto a stalenowcaptured earlier makesnow() >= dttrue at the DB, so update should pass. To test rejection, setdtto a future instant.Apply:
- // violates `dt <= now()` - await expect(db.foo.update({ where: { id: '1' }, data: { dt: now } })).toBeRejectedNotFound(); + // violates `now() >= dt` by setting dt to the future + await expect( + db.foo.update({ where: { id: '1' }, data: { dt: new Date(now.getTime() + 60_000) } }) + ).toBeRejectedNotFound();packages/runtime/src/client/crud/dialects/sqlite.ts (1)
39-56: Typed-JSON detection is incorrect (BuiltinType can’t be in schema.typeDefs)
typehere is BuiltinType, sotype in this.schema.typeDefsis always false. Also the special-casing is redundant because Json is already handled below.- if (this.schema.typeDefs && type in this.schema.typeDefs) { - // typed JSON field - return JSON.stringify(value); - } else { - return match(type) + return match(type) .with('Boolean', () => (value ? 1 : 0)) .with('DateTime', () => value instanceof Date ? value.toISOString() : typeof value === 'string' ? new Date(value).toISOString() : value, ) .with('Decimal', () => (value as Decimal).toString()) .with('Bytes', () => Buffer.from(value as Uint8Array)) .with('Json', () => JSON.stringify(value)) .otherwise(() => value); - }packages/runtime/src/client/functions.ts (1)
65-75: Array operators are Postgres‑specific; break on SQLite
@>and&&won’t work on SQLite. Either branch by provider or fail fast with a clear error until a JSON1‑based implementation is added.-export const has: ZModelFunction<any> = (eb, args) => { - const [field, search] = args; +export const has: ZModelFunction<any> = (eb, args, { dialect }) => { + const [field, search] = args; if (!field) { throw new Error('"field" parameter is required'); } if (!search) { throw new Error('"search" parameter is required'); } - return eb(field, '@>', [search]); + if (dialect.provider === 'postgresql') { + return eb(field, '@>', [search]); + } + throw new Error('"has" is not supported on this dialect yet'); }; @@ -export const hasSome: ZModelFunction<any> = (eb, args) => { - const [field, search] = args; +export const hasSome: ZModelFunction<any> = (eb, args, { dialect }) => { + const [field, search] = args; if (!field) { throw new Error('"field" parameter is required'); } if (!search) { throw new Error('"search" parameter is required'); } - return eb(field, '&&', search); + if (dialect.provider === 'postgresql') { + return eb(field, '&&', search); + } + throw new Error('"hasSome" is not supported on this dialect yet'); };If you want SQLite support now, I can help wire JSON1 EXISTS/INSTR patterns for membership checks.
Also applies to: 87-96
🧹 Nitpick comments (13)
packages/runtime/test/policy/policy-functions.test.ts (5)
5-24: Default case-sensitive test is correct; tweak naming and provider-check phrasing
- Rename the test to clarify it verifies the default (implicit) behavior.
- The comment “sqlite is always case-insensitive” is over-broad; SQLite’s LIKE/INSTR are ASCII‑case‑insensitive by default. Consider clarifying.
- Optional: factor the provider check into a local
isSQLitefor readability.Apply:
-it('supports contains case-sensitive', async () => { +it('supports contains case-sensitive (default)', async () => { @@ - await expect(db.foo.create({ data: { string: 'bcd' } })).toBeRejectedByPolicy(); - if (db.$schema.provider.type === 'sqlite') { - // sqlite is always case-insensitive + await expect(db.foo.create({ data: { string: 'bcd' } })).toBeRejectedByPolicy(); + const isSQLite = db.$schema.provider.type === 'sqlite'; + if (isSQLite) { + // SQLite LIKE/INSTR are ASCII-case-insensitive by default await expect(db.foo.create({ data: { string: 'Acd' } })).toResolveTruthy(); } else { await expect(db.foo.create({ data: { string: 'Acd' } })).toBeRejectedByPolicy(); }
26-46: Explicit case-sensitive mirrors default; reduce duplicationThese assertions duplicate the prior test with the explicit flag. Consider
describe.eachto parameterize (implicit vs explicit) and collapse duplication.Example:
describe.each([ { name: 'default', expr: "contains(string, 'a')" }, { name: 'explicit', expr: "contains(string, 'a', false)" }, ])('supports contains case-sensitive: %s', ({ expr }) => { // build schema with `${expr}` and run the same expectations });
80-85: SQLite note: ASCII-only case-insensitivity and PRAGMA caveatIf tests ever use non-ASCII letters or
PRAGMA case_sensitive_like = ON, expectations will differ. Add a brief note or guard to prevent future false failures.Would you like me to add a tiny regression test using a non-ASCII character to document this?
141-144: Duplicate assertion; remove the extra lineLine 142 repeats Line 141 and adds no value. Drop it.
Apply:
await expect(anonDb.foo.create({ data: {} })).toBeRejectedByPolicy(); - await expect(anonDb.foo.create({ data: {} })).toBeRejectedByPolicy();
233-233: Remove console.log from testsStray logging adds noise to CI output.
Apply:
- console.log(created);packages/language/res/stdlib.zmodel (5)
126-129: Doc: default matching isn't universally case-sensitive; clarify dialect behavior.SQLite/MySQL default LIKE may be case-insensitive depending on collation; Postgres LIKE is case-sensitive. Recommend wording that reflects provider/collation variance and mentions the CI emulation strategy. Also fix parameter spelling in docs.
Apply:
- * Checks if the field value contains the search string. By default, the search is case-sensitive, and - * "LIKE" operator is used to match. If `caseInSensitive` is true, "ILIKE" operator is used if - * supported, otherwise it still falls back to "LIKE" and delivers whatever the database's - * behavior is. + * Checks if the field value contains the search string. By default, matching follows the database's + * collation and LIKE semantics. If `caseInsensitive` is true, use "ILIKE" where supported (e.g. PostgreSQL); + * otherwise use a dialect-specific best‑effort (e.g. LOWER(field) LIKE LOWER(search)) or fall back to LIKE, + * so behavior may vary by provider/collation.
141-145: Doc: mirror the dialect-aware wording for startsWith.Same concerns as contains(); also fix param spelling.
- * Checks the field value starts with the search string. By default, the search is case-sensitive, and - * "LIKE" operator is used to match. If `caseInSensitive` is true, "ILIKE" operator is used if - * supported, otherwise it still falls back to "LIKE" and delivers whatever the database's - * behavior is. + * Checks the field value starts with the search string. By default, matching follows the database's + * collation and LIKE semantics. If `caseInsensitive` is true, use "ILIKE" where supported; otherwise apply + * a dialect-specific best‑effort or fall back to LIKE, so behavior may vary by provider/collation.
146-147: Parameter name: usecaseInsensitive.-function startsWith(field: String, search: String, caseInSensitive: Boolean?): Boolean { +function startsWith(field: String, search: String, caseInsensitive: Boolean?): Boolean {
150-154: Doc: mirror the dialect-aware wording for endsWith.And fix param spelling.
- * Checks if the field value ends with the search string. By default, the search is case-sensitive, and - * "LIKE" operator is used to match. If `caseInSensitive` is true, "ILIKE" operator is used if - * supported, otherwise it still falls back to "LIKE" and delivers whatever the database's - * behavior is. + * Checks if the field value ends with the search string. By default, matching follows the database's + * collation and LIKE semantics. If `caseInsensitive` is true, use "ILIKE" where supported; otherwise apply + * a dialect-specific best‑effort or fall back to LIKE, so behavior may vary by provider/collation.
155-156: Parameter name: usecaseInsensitive.-function endsWith(field: String, search: String, caseInSensitive: Boolean?): Boolean { +function endsWith(field: String, search: String, caseInsensitive: Boolean?): Boolean {packages/runtime/test/client-api/relation/self-relation.test.ts (1)
489-505: Clarify semantics:deleteManyon M2M deletes records, not just disconnectsThe comment says “disconnect and delete,” but nested
deleteManyunder a M2M relation deletes the matched related records themselves (thus removing all of their relations). The subsequent assertions depend on this (David deleted, Eve remains). Update the comment to avoid confusion.- // Test deleteMany operation (disconnect and delete) + // Test deleteMany operation (delete related records; not just disconnect)packages/runtime/src/client/crud/dialects/base-dialect.ts (1)
1262-1271: New dialect APIs are not yet applied in base string filtersYou added getStringCasingBehavior but buildStringFilter (Lines 650–703) still hardcodes ILIKE/LIKE without consulting it. Consider delegating casing to the dialect to avoid SQLite incompatibilities and keep behavior consistent with functions.ts.
- .with('contains', () => - mode === 'insensitive' - ? eb(fieldRef, 'ilike', sql.val(`%${value}%`)) - : eb(fieldRef, 'like', sql.val(`%${value}%`)), - ) + .with('contains', () => this.buildLike(eb, fieldRef, `%${value}%`, mode))And add a small helper in this class that uses getStringCasingBehavior to pick operator or lower both sides.
packages/runtime/src/client/crud/dialects/sqlite.ts (1)
118-128: Surface better error context for JSON parse failuresMinor: include dialect/source in the message to aid triage.
- } catch (e) { - throw new QueryError('Invalid JSON returned', e); + } catch (e) { + throw new QueryError('Invalid JSON returned from SQLite', e); }
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (12)
packages/language/res/stdlib.zmodel(2 hunks)packages/runtime/src/client/crud/dialects/base-dialect.ts(2 hunks)packages/runtime/src/client/crud/dialects/postgresql.ts(5 hunks)packages/runtime/src/client/crud/dialects/sqlite.ts(5 hunks)packages/runtime/src/client/functions.ts(5 hunks)packages/runtime/src/client/options.ts(1 hunks)packages/runtime/test/client-api/relation/many-to-many.test.ts(3 hunks)packages/runtime/test/client-api/relation/one-to-many.test.ts(3 hunks)packages/runtime/test/client-api/relation/self-relation.test.ts(6 hunks)packages/runtime/test/plugin/on-kysely-query.test.ts(1 hunks)packages/runtime/test/policy/migrated/nested-to-many.test.ts(1 hunks)packages/runtime/test/policy/policy-functions.test.ts(3 hunks)
🚧 Files skipped from review as they are similar to previous changes (4)
- packages/runtime/test/policy/migrated/nested-to-many.test.ts
- packages/runtime/test/client-api/relation/many-to-many.test.ts
- packages/runtime/test/plugin/on-kysely-query.test.ts
- packages/runtime/test/client-api/relation/one-to-many.test.ts
🧰 Additional context used
📓 Path-based instructions (1)
{packages,samples,tests}/**
📄 CodeRabbit inference engine (CLAUDE.md)
Place packages only under
packages/,samples/, ortests/
Files:
packages/runtime/src/client/crud/dialects/base-dialect.tspackages/runtime/test/client-api/relation/self-relation.test.tspackages/runtime/test/policy/policy-functions.test.tspackages/runtime/src/client/crud/dialects/sqlite.tspackages/runtime/src/client/functions.tspackages/runtime/src/client/options.tspackages/language/res/stdlib.zmodelpackages/runtime/src/client/crud/dialects/postgresql.ts
🧬 Code graph analysis (6)
packages/runtime/src/client/crud/dialects/base-dialect.ts (1)
packages/sdk/src/schema/schema.ts (1)
FieldDef(60-75)
packages/runtime/test/client-api/relation/self-relation.test.ts (1)
packages/runtime/test/utils.ts (1)
createTestClient(50-166)
packages/runtime/test/policy/policy-functions.test.ts (1)
packages/runtime/test/policy/utils.ts (1)
createPolicyTestClient(14-25)
packages/runtime/src/client/crud/dialects/sqlite.ts (3)
packages/sdk/src/schema/schema.ts (2)
BuiltinType(85-95)FieldDef(60-75)packages/common-helpers/src/tiny-invariant.ts (1)
invariant(4-14)packages/runtime/src/client/errors.ts (1)
QueryError(13-17)
packages/runtime/src/client/functions.ts (2)
packages/runtime/src/client/options.ts (2)
ZModelFunction(36-40)ZModelFunctionContext(9-34)packages/common-helpers/src/tiny-invariant.ts (1)
invariant(4-14)
packages/runtime/src/client/crud/dialects/postgresql.ts (4)
packages/runtime/src/client/options.ts (1)
ClientOptions(45-90)packages/sdk/src/schema/schema.ts (2)
BuiltinType(85-95)FieldDef(60-75)packages/common-helpers/src/tiny-invariant.ts (1)
invariant(4-14)packages/runtime/src/client/errors.ts (1)
QueryError(13-17)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: build-test (20.x, postgresql)
- GitHub Check: build-test (20.x, sqlite)
🔇 Additional comments (15)
packages/runtime/test/policy/policy-functions.test.ts (1)
47-60: Case-insensitive branch looks goodCovers negative and positive paths succinctly. No issues.
packages/language/res/stdlib.zmodel (1)
131-132: Rename parametercaseInSensitive→caseInsensitiveFix spelling in stdlib: update the parameter name and docs in packages/language/res/stdlib.zmodel (functions: contains, startsWith, endsWith — occurrences around lines 127–155) and update any call sites; verify with: rg -nP '\bcaseInSensitive\b' -g '!/node_modules/' and rebuild to confirm no breakage.
packages/runtime/test/client-api/relation/self-relation.test.ts (2)
4-9: Good isolation and cleanup patternPer‑test client creation with an afterEach disconnect is solid and keeps DB state isolated across the new dual‑provider matrix.
166-170: Make assertions order‑agnostic (use expect.arrayContaining + assert lengths)Several assertions in packages/runtime/test/client-api/relation/self-relation.test.ts assume array ordering; convert them to order‑agnostic checks with expect.arrayContaining and add explicit .toHaveLength where exact cardinality is required. Affected locations: 166–170, 329–331, 359–361, 372, 444–445, 693–707 — the suggested diffs in the original comment are applicable. Verification couldn't be completed here: the runner failed with "Command 'vitest' not found". Re-run verification for both providers with a working test command (for example: TEST_DB_PROVIDER=sqlite pnpm test packages/runtime/test/client-api/relation/self-relation.test.ts and TEST_DB_PROVIDER=postgresql pnpm test packages/runtime/test/client-api/relation/self-relation.test.ts) and paste the outputs.
packages/runtime/src/client/crud/dialects/base-dialect.ts (1)
48-51: Baseline transformOutput looks goodNo-op default is fine and keeps existing behavior for dialects that don’t override.
packages/runtime/src/client/crud/dialects/sqlite.ts (3)
412-415: Casing behavior mapping LGTMSQLite: no ILIKE; LIKE is case-insensitive by default (ASCII). Matches implementation.
60-76: Same typed-JSON issue on output; also redundant branchKeep the null/undefined guard. Remove the
schema.typeDefscheck and rely on the BuiltinType dispatch (Json branch).- override transformOutput(value: unknown, type: BuiltinType) { + override transformOutput(value: unknown, type: BuiltinType) { if (value === null || value === undefined) { return value; - } else if (this.schema.typeDefs && type in this.schema.typeDefs) { - // typed JSON field - return this.transformOutputJson(value); - } else { - return match(type) + } else { + return match(type) .with('Boolean', () => this.transformOutputBoolean(value)) .with('DateTime', () => this.transformOutputDate(value)) .with('Bytes', () => this.transformOutputBytes(value)) .with('Decimal', () => this.transformOutputDecimal(value)) .with('BigInt', () => this.transformOutputBigInt(value)) .with('Json', () => this.transformOutputJson(value)) .otherwise(() => super.transformOutput(value, type)); } }
382-410: Change SQLite DateTime & Json SQL types to textSQLite dialect serializes DateTime to ISO strings and JSON via JSON.stringify, so getFieldSqlType should return 'text' instead of 'numeric'/'jsonb'. Update packages/runtime/src/client/crud/dialects/sqlite.ts (getFieldSqlType, ~lines 382–410). Repo search shows no migrations referencing 'jsonb' (postgres dialect and schema-db-pusher still use jsonb), so this change is safe.
packages/runtime/src/client/crud/dialects/postgresql.ts (5)
65-75: Output transformer dispatch LGTMCovers DateTime/Bytes/BigInt/Decimal and defers others to base.
99-111: No mutation of Date inputs — good; confirm default option behaviorThe clone-and-adjust pattern avoids side effects; timezone fix is gated by
!== false, matching the option’s “default true” semantics.
437-468: Field SQL type mapping mostly good; revisit DateTime choice
'timestamp'vs'timestamptz'can affect node‑pg parsing and timezone semantics. If tests rely on UTC semantics, consider'timestamptz'. Otherwise, keep'timestamp'and rely on the fixPostgresTimezone path.Can you confirm which type your migrations generate for DateTime today?
470-473: Casing behavior mapping LGTMPostgres supports ILIKE; LIKE is case‑sensitive — matches implementation.
29-31: Constructor signature change — verified
Found instantiation in packages/runtime/src/client/crud/dialects/index.ts using new PostgresCrudDialect(schema, options).packages/runtime/src/client/functions.ts (1)
143-150: Validate boolean literal strictly in readBooleanAvoid truthy coercion of non-boolean literals (e.g., 'false' string).
function readBoolean(expr: Expression<any> | undefined, defaultValue: boolean) { if (expr === undefined) { return defaultValue; } const opNode = expr.toOperationNode(); - invariant(ValueNode.is(opNode), 'expression must be a literal value'); - return !!opNode.value; + invariant(ValueNode.is(opNode) && typeof opNode.value === 'boolean', '"caseInsensitive" must be a boolean literal'); + return opNode.value; }packages/runtime/src/client/options.ts (1)
66-75: Option semantics: default-to-true OK — propagation not verified; confirm forwarding to dialectpostgresql dialect uses this.options.fixPostgresTimezone; ripgrep shows matches only in packages/runtime/src/client/options.ts and packages/runtime/src/client/crud/dialects/postgresql.ts. No occurrences in ResultProcessor or ClientImpl that forward client options into the dialect. Verify ClientImpl/ResultProcessor pass client options (including fixPostgresTimezone) into the dialect instance (e.g., constructor/new call or assigned options).
|
Claude finished @ymc9's task —— View job PR Review: Database Compatibility FixesI've completed a comprehensive review of this PR focused on dual database compatibility. Here's my detailed analysis: Todo List:
SummaryThis PR successfully implements dual database compatibility for PostgreSQL and SQLite with extensive test coverage. The changes are well-structured and address key differences between the database providers. Code Quality & Best Practices ✅Positive Aspects:
Areas for Improvement:
Database Dialect Implementation 🎯PostgreSQL Dialect (
|
|
Claude finished @ymc9's task —— View job PR Review: Database Compatibility FixesI've completed a comprehensive review of this PR focused on dual database compatibility. Here's my detailed analysis: Todo List:
SummaryThis PR successfully implements dual database compatibility for PostgreSQL and SQLite with extensive test coverage. The changes are well-structured and address key differences between the database providers. Code Quality & Best Practices ✅Positive Aspects:
Areas for Improvement:
Database Dialect Implementation 🎯PostgreSQL Dialect (
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
packages/runtime/src/client/crud/operations/base.ts (1)
1630-1646: Bug: early return aborts remaining disconnects in m2m loopIf one target isn’t found, the function returns and skips subsequent items. Use continue to ignore the missing one but proceed with others.
Apply:
- for (const d of disconnectConditions) { + for (const d of disconnectConditions) { const ids = await this.getEntityIds(kysely, model, d); if (!ids) { // not found - return; + continue; } await this.handleManyToManyRelation( kysely, 'disconnect', fromRelation.model, fromRelation.field, fromRelation.ids, m2m.otherModel, m2m.otherField, ids, m2m.joinTable, ); }
🧹 Nitpick comments (4)
packages/runtime/src/client/crud/operations/base.ts (4)
365-367: Sequential post-create relation handling: OK, but watch for perf regressionsSwitching from batched to sequential awaits improves determinism, but can slow large nested create trees. If DB constraints don’t require strict ordering, consider a bounded-concurrency approach.
Example (conceptual):
- for (const [field, subPayload] of Object.entries(postCreateRelations)) { - await this.processNoneOwnedRelationForCreate(kysely, model, field, subPayload, createdEntity); - } + for (const entries of chunk(Object.entries(postCreateRelations), 8)) { + await Promise.all(entries.map(([field, subPayload]) => + this.processNoneOwnedRelationForCreate(kysely, model, field, subPayload, createdEntity) + )); + }
1365-1367: Sequentializing nested updates: consistent, but consider bounded concurrency for large batchesThe move from Promise.all to for-await avoids interleaving and simplifies error surfaces. For high-cardinality relations, a small concurrency pool (e.g., 8–16) often yields better throughput without stressing the DB.
Also applies to: 1373-1379, 1384-1384, 1389-1389, 1394-1394, 1401-1401, 1405-1417, 1422-1441, 1446-1449, 1454-1454, 1459-1459
1591-1598: Avoid redundant read on connectOrCreateYou already have the existing entity’s ids from exists; pass them directly to connectRelation to skip another lookup.
- for (const { where, create } of _data) { - const existing = await this.exists(kysely, model, where); - if (existing) { - await this.connectRelation(kysely, model, [where], fromRelation); - } else { - await this.create(kysely, model, create, fromRelation); - } - } + for (const { where, create } of _data) { + const existing = await this.exists(kysely, model, where); + if (existing) { + await this.connectRelation(kysely, model, existing, fromRelation); + } else { + await this.create(kysely, model, create, fromRelation); + } + }
597-612: processNoneOwnedRelationForCreate: tighten type; transaction wrapping verified
- Type nit — align cast with createMany's signature:
- subPayload as { data: any; skipDuplicates: boolean }, + subPayload as { data: any; skipDuplicates?: boolean },
- Transaction check — verified top-level create/createMany are executed inside safeTransaction and pass the tx/kysely down (packages/runtime/src/client/crud/operations/create.ts → create(tx, ...)); relation handlers accept a kysely/tx (packages/runtime/src/client/crud/operations/base.ts), so connectOrCreate runs under the same transaction and the TOCTOU concern is mitigated.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
packages/runtime/src/client/crud/operations/base.ts(10 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
{packages,samples,tests}/**
📄 CodeRabbit inference engine (CLAUDE.md)
Place packages only under
packages/,samples/, ortests/
Files:
packages/runtime/src/client/crud/operations/base.ts
🧬 Code graph analysis (1)
packages/runtime/src/client/crud/operations/base.ts (3)
packages/runtime/src/utils/enumerate.ts (1)
enumerate(9-17)packages/common-helpers/src/tiny-invariant.ts (1)
invariant(4-14)packages/runtime/src/client/errors.ts (1)
NotFoundError(27-31)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: claude-review
- GitHub Check: build-test (20.x, sqlite)
- GitHub Check: build-test (20.x, postgresql)
🔇 Additional comments (2)
packages/runtime/src/client/crud/operations/base.ts (2)
1488-1506: Guard m2m connect against missing targets; defer NotFound to post-check (keep prior semantics)Throwing immediately on the first missing target changes behavior and skips validating the rest. Prefer pushing undefined and using the existing count check to raise NotFound once, after attempting all.
Apply:
- for (const d of _data) { - const ids = await this.getEntityIds(kysely, model, d); - if (!ids) { - throw new NotFoundError(model); - } - const r = await this.handleManyToManyRelation( + for (const d of _data) { + const ids = await this.getEntityIds(kysely, model, d); + if (!ids) { + results.push(undefined); + continue; + } + const r = await this.handleManyToManyRelation( kysely, 'connect', fromRelation.model, fromRelation.field, fromRelation.ids, m2m.otherModel!, m2m.otherField!, ids, m2m.joinTable, ); results.push(r); }Also applies to: 1491-1494
1735-1754: Guard m2m set against missing targets; rely on post-loop NotFound checkSame as connect: don’t throw immediately on first miss. Push undefined and let the existing length check raise NotFound. This keeps behavior consistent and evaluates all inputs.
- const results: (unknown | undefined)[] = []; - for (const d of _data) { - const ids = await this.getEntityIds(kysely, model, d); - if (!ids) { - throw new NotFoundError(model); - } - results.push( + const results: (unknown | undefined)[] = []; + for (const d of _data) { + const ids = await this.getEntityIds(kysely, model, d); + if (!ids) { + results.push(undefined); + continue; + } + results.push( await this.handleManyToManyRelation( kysely, 'connect', fromRelation.model, fromRelation.field, fromRelation.ids, m2m.otherModel, m2m.otherField, ids, m2m.joinTable, ), ); }Also applies to: 1738-1744
|
Claude finished @ymc9's task —— View job PR Review: Database Compatibility FixesI've completed a comprehensive review of this PR focused on dual database compatibility. Here's my detailed analysis: Todo List:
SummaryThis PR successfully implements dual database compatibility for PostgreSQL and SQLite with extensive test coverage. The changes are well-structured and address key differences between the database providers. Code Quality & Best Practices ✅Positive Aspects:
Areas for Improvement:
Database Dialect Implementation 🎯PostgreSQL Dialect (
|
|
Claude finished @ymc9's task —— View job PR Review: Database Compatibility Fixes
|
|
Claude finished @ymc9's task —— View job PR Review: Database Compatibility FixesI've completed a comprehensive review of this PR focused on dual database compatibility. Here's my detailed analysis: Todo List:
SummaryThis PR successfully implements dual database compatibility for PostgreSQL and SQLite with extensive test coverage. The changes are well-structured and address key differences between the database providers. Code Quality & Best Practices ✅Positive Aspects:
Areas for Improvement:
Database Dialect Implementation 🎯PostgreSQL Dialect (
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 0
🧹 Nitpick comments (4)
packages/language/test/delegate.test.ts (1)
8-12: DRY the datasource and make provider/url configurableSame note as mixin tests: inline sqlite blocks are fine but duplicate and make pg runs less representative. Replace with a shared
dssnippet.- datasource db { - provider = 'sqlite' - url = 'file:./dev.db' - } + ${ds}If you prefer not to introduce
${ds}interpolation, alternatively wrap fixtures:// utils export const withDatasource = (s: string) => `${ds}\n${s}`; // usage const model = await loadSchema(withDatasource(` model A { ... } `));Also applies to: 33-36, 54-57, 76-79, 98-101
packages/language/test/import.test.ts (2)
15-19: Use shared datasource snippet to keep imports fixture provider‑agnosticReplacing the literal block with
${ds}keeps this test neutral across pg/sqlite.-datasource db { - provider = 'sqlite' - url = 'file:./dev.db' -} +${ds}
57-61: Same refactor for other fixtures in this fileApply the same
${ds}substitution to avoid drift between fixtures and support the CI matrix cleanly.Also applies to: 80-84
packages/language/test/mixin.test.ts (1)
8-12: Replace hardcoded sqlite datasource with a shared env-driven snippetMultiple tests embed identical datasource blocks with provider = 'sqlite'. Replace them with a shared, env-driven
dssnippet to support a sqlite/postgres test matrix and reduce duplication.- datasource db { - provider = 'sqlite' - url = 'file:./dev.db' - } + ${ds}Add once per file (or export from a shared test util):
// test datasource snippet (env-driven; parsing only, no connect) const ds = ` datasource db { provider = '${process.env.TEST_DB_PROVIDER ?? 'sqlite'}' url = '${process.env.TEST_DB_URL ?? 'file:./dev.db'}' } `;Locations to update:
- packages/language/test/mixin.test.ts — blocks at lines ~8–12, 33–37, 65–69
- packages/language/test/import.test.ts — blocks at lines ~15–18, 57–60, 80–83
- packages/language/test/delegate.test.ts — blocks at lines ~8–11, 32–35, 53–56, 75–78, 97–100
- packages/language/test/expression-validation.test.ts — block at lines ~57–60
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
packages/language/test/delegate.test.ts(5 hunks)packages/language/test/import.test.ts(3 hunks)packages/language/test/mixin.test.ts(3 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
{packages,samples,tests}/**
📄 CodeRabbit inference engine (CLAUDE.md)
Place packages only under
packages/,samples/, ortests/
Files:
packages/language/test/mixin.test.tspackages/language/test/import.test.tspackages/language/test/delegate.test.ts
🧠 Learnings (1)
📚 Learning: 2025-09-04T12:38:14.150Z
Learnt from: CR
PR: zenstackhq/zenstack-v3#0
File: CLAUDE.md:0-0
Timestamp: 2025-09-04T12:38:14.150Z
Learning: Applies to **/schema.zmodel : Name ZModel schema files `schema.zmodel`
Applied to files:
packages/language/test/import.test.ts
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: build-test (20.x, sqlite)
- GitHub Check: build-test (20.x, postgresql)
- GitHub Check: claude-review
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 1
🧹 Nitpick comments (2)
packages/language/test/import.test.ts (2)
70-70: Avoid index-based lookup; select model/field by nameIndexing into
model.declarationsandfieldsis fragile with added blocks. Use name-based lookup.Apply this diff:
- expect((model.declarations[1] as DataModel).fields[1].type.reference?.ref?.name).toBe('Role'); + const user = model.declarations.filter(isDataModel).find((d) => d.name === 'User')!; + const roleField = user.fields.find((f) => f.name === 'role')!; + expect(roleField.type.reference?.ref?.name).toBe('Role');
106-106: Same brittleness here; prefer name-based selectionSelecting by index can break as declarations reorder. Use model/field names.
Apply this diff:
- expect((modelA.declarations[1] as DataModel).fields[1].type.reference?.ref?.name).toBe('B'); + const aModel = modelA.declarations.filter(isDataModel).find((d) => d.name === 'A') as DataModel; + const bField = aModel.fields.find((f) => f.name === 'b')!; + expect(bField.type.reference?.ref?.name).toBe('B');
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
packages/language/test/import.test.ts(5 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
{packages,samples,tests}/**
📄 CodeRabbit inference engine (CLAUDE.md)
Place packages only under
packages/,samples/, ortests/
Files:
packages/language/test/import.test.ts
🧠 Learnings (1)
📚 Learning: 2025-09-04T12:38:14.150Z
Learnt from: CR
PR: zenstackhq/zenstack-v3#0
File: CLAUDE.md:0-0
Timestamp: 2025-09-04T12:38:14.150Z
Learning: Applies to **/schema.zmodel : Name ZModel schema files `schema.zmodel`
Applied to files:
packages/language/test/import.test.ts
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: build-test (20.x, sqlite)
- GitHub Check: build-test (20.x, postgresql)
🔇 Additional comments (2)
packages/language/test/import.test.ts (2)
15-19: OK to add sqlite datasource to fixtureMakes the merged document valid; no issues spotted here.
80-84: OK to add sqlite datasource for cyclic-import fixtureNo functional concerns; keeps schema valid without duplicating datasource blocks.
Please confirm that only one datasource exists per merged graph in these fixtures to avoid loader ambiguity.

Summary by CodeRabbit
New Features
Bug Fixes
Refactor
Tests / CI